Conversation
There was a problem hiding this comment.
Functionally, this looks great... exactly what we discussed. However, you can drastically reduce the test method code duplication to improve maintenance, etc. Create a method like:
private void testUIOptionalFeatures(String linkText, OptionalFeatureType type, List<String> featureIds)
{
goToAdminConsole();
waitAndClickAndWait(Locator.linkWithText(linkText));
var optionalFeaturesPage = new OptionalFeaturesPage(getDriver());
...
}
The two test methods each then become a single line that calls this method.
| import java.util.Set; | ||
|
|
||
| /* | ||
| Wraps Optional Features, Experimental Featueres, Deprecated Features pages linked |
There was a problem hiding this comment.
| Wraps Optional Features, Experimental Featueres, Deprecated Features pages linked | |
| Wraps Optional Features, Experimental Features, Deprecated Features pages linked |
labkey-adam
left a comment
labkey-adam
left a comment
There was a problem hiding this comment.
Not a big deal, but if you passed in the link text to the shared method, you could avoid a couple more lines of redundant code. The test*() methods could literally be one-liners.
Also, not a big deal... but in my review I had added a suggestion to fix a comment typo.
| @@ -247,7 +247,7 @@ public void testUIExperimentalFeatures() | |||
| waitAndClickAndWait(Locator.linkWithText("experimental features")); | |||
There was a problem hiding this comment.
Should remove this line as well
labkey-danield
left a comment
labkey-danield
left a comment
There was a problem hiding this comment.
The getListItems method should be moved out of the ElementCache.
| public final Locator listItemLabelLoc = Locator.tagWithClass("div", "list-group-item") | ||
| .child(Locator.tag("Label")); | ||
| private Map<String, String> _listItems; | ||
|
|
||
| public Map<String, String> getListItems() | ||
| { | ||
| if (_listItems == null) { | ||
| _listItems = new HashMap<String, String>(); | ||
| for (WebElement el : listItemLabelLoc.findElements(listGroupElement)) { | ||
| WebElement idEl = Locator.tagWithAttribute("type", "checkbox") | ||
| .findElement(el); | ||
| WebElement labelEl = Locator.tagWithClass("span", "toggle-label-text") | ||
| .findElement(el); | ||
| _listItems.put(idEl.getText(), labelEl.getText()); | ||
| } | ||
| } | ||
| return _listItems; | ||
| } |
There was a problem hiding this comment.
This doesn't belong in the ElementCache, it should be broken out to a private method in the class. The Locator doesn't need to be public it is used only in the getListItems method.
There was a problem hiding this comment.
Actually this code can be removed altogether. The method getListItems is only called by getFeatureMap() which is only called by getFeatureIds(), which is not called.
This method never gets executed.
labkey-danield
left a comment
labkey-danield
left a comment
There was a problem hiding this comment.
You have unused code in OptionalFeaturesPage that should be removed.
| public final Locator listItemLabelLoc = Locator.tagWithClass("div", "list-group-item") | ||
| .child(Locator.tag("Label")); | ||
| private Map<String, String> _listItems; | ||
|
|
||
| public Map<String, String> getListItems() | ||
| { | ||
| if (_listItems == null) { | ||
| _listItems = new HashMap<String, String>(); | ||
| for (WebElement el : listItemLabelLoc.findElements(listGroupElement)) { | ||
| WebElement idEl = Locator.tagWithAttribute("type", "checkbox") | ||
| .findElement(el); | ||
| WebElement labelEl = Locator.tagWithClass("span", "toggle-label-text") | ||
| .findElement(el); | ||
| _listItems.put(idEl.getText(), labelEl.getText()); | ||
| } | ||
| } | ||
| return _listItems; | ||
| } |
There was a problem hiding this comment.
Actually this code can be removed altogether. The method getListItems is only called by getFeatureMap() which is only called by getFeatureIds(), which is not called.
This method never gets executed.
3 participants
Rationale
This change adds UI coverage for optional features and experimental features.
Recent changes have refactored the implementation for exp features, optional features, and deprecated features under the singular optionalFeaturesService implementation. This coverage seeks to validate the UI continues to work as expected.
Related Pull Requests
LabKey/platform#6828
Changes
New page class to wrap all three of the optional features pages